Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't open the wallet window after closing deposit flow #877

Merged
merged 8 commits into from
Dec 2, 2024

Conversation

kkosiorowska
Copy link
Contributor

@kkosiorowska kkosiorowska commented Nov 19, 2024

Closes #863

The transaction sign window appears despite the rejection of the deposit. Let's already use the solution for the withdrawal flow. The cancel logic was moved to useCancelPromise.

Screen.Recording.2024-11-19.at.15.58.30.mov

The transaction sign window appears despite the rejection of the deposit. Let's make sure that the user does not close the window when the wallet opening action has already been triggered.
@kkosiorowska kkosiorowska self-assigned this Nov 19, 2024
Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for acre-dapp ready!

Name Link
🔨 Latest commit 1ff365d
🔍 Latest deploy log https://app.netlify.com/sites/acre-dapp/deploys/674db656fe9ad10008acb091
😎 Deploy Preview https://deploy-preview-877--acre-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for acre-dapp-testnet ready!

Name Link
🔨 Latest commit 1ff365d
🔍 Latest deploy log https://app.netlify.com/sites/acre-dapp-testnet/deploys/674db656a038fc00087cd6d6
😎 Deploy Preview https://deploy-preview-877--acre-dapp-testnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

To be able to cancel the deposit/withdrawal flow when a user closes the modal.
Cancel the withdrawal/deposit transaction when the user closes the flow modal.
@kkosiorowska kkosiorowska marked this pull request as ready for review November 19, 2024 14:59
@kkosiorowska kkosiorowska marked this pull request as draft November 19, 2024 15:12
@kkosiorowska kkosiorowska marked this pull request as ready for review November 20, 2024 15:49
@kkosiorowska kkosiorowska marked this pull request as draft November 21, 2024 09:32
@kkosiorowska kkosiorowska marked this pull request as ready for review November 25, 2024 09:12
Previously, `shouldOpenErrorModal` returned a wrong value. Let's check the shouldOpenErrorModal value using the current sessionId to make sure the data is up to date.
Comment on lines +32 to +36
const sessionId = useRef(Math.random())
const { cancel, resolve, sessionIdToPromise } = useCancelPromise(
sessionId.current,
"Deposit cancelled",
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can generate sessionId inside useCancelPromise hook?

Copy link
Contributor Author

@kkosiorowska kkosiorowska Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously I used this solution but had the problem that shouldOpenErrorModal was not returning the correct value for the right session.

Steps to reproduce:

  1. Reject the withdrawal request.
  2. Try again to do the withdrawal.

The problem was that the wallet window appeared and the dApp displayed a modal error. This happened because shouldOpenErrorModal was true. Hook didn't return the right value for the session.

Screenshot 2024-11-26 at 15 13 29 Screenshot 2024-11-26 at 15 24 42

I decided to pass sessionId to the hook to make sure we get the right value for shouldOpenErrorModal. This solution solved the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we can hide the sessionIdToPromise in useCancelPromise hook and generate the session id here. For example:
useCancelPromise.ts

const sessionId = ... //generate session here

... // Implementation here
 
return {
 cancel,
 resolve,
 options: { shouldOpenErrorModal: false  } 
} 

@kkosiorowska kkosiorowska force-pushed the fix-wallet-window branch 2 times, most recently from b8e57d9 to d196200 Compare November 26, 2024 14:15
r-czajkowski
r-czajkowski previously approved these changes Nov 28, 2024
@r-czajkowski r-czajkowski merged commit 2b7ead0 into main Dec 2, 2024
26 of 28 checks passed
@r-czajkowski r-czajkowski deleted the fix-wallet-window branch December 2, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The transaction sign window appears despite the rejection of the deposit
2 participants